Découpe la page détail des indicateurs avec des onglets #3521
Conversation
mariheck
force-pushed
the
TET-4241/onglets-indicateurs
branch
from
c4273ea to
ac45a2d
Compare
| <Tab label={`${enfants.length} Sous indicateurs`}> | ||
| <SousIndicateurs definition={definition} enfantsIds={enfants} /> | ||
| </Tab> | ||
| ) : undefined} |
There was a problem hiding this comment.
Pourquoi mettre undefined plutôt que rien ?
There was a problem hiding this comment.
Si je fais un simple condition === true && <Tab> ... </Tab>, je vais avoir une erreur du composant Tabs car il reçoit des enfants de type Element | boolean au lieu de Element | undefined. Et si j'écris quelque chose comme condition === true ? <Tab> ... </Tab> : <></> l'affichage des onglets est cassé avec un onglet vide qui apparait.
| {isDeleteModalOpen && ( | ||
| <Modal | ||
| openState={{ | ||
| isOpen: isDeleteModalOpen, | ||
| setIsOpen: setIsDeleteModalOpen, | ||
| }} | ||
| title="Suppression de l'indicateur" | ||
| subTitle={definition.titre} | ||
| description="Êtes-vous sûr de vouloir supprimer cet indicateur personnalisé ? Vous perdrez définitivement les données associées à cet indicateur." | ||
| renderFooter={({ close }) => ( | ||
| <ModalFooterOKCancel | ||
| btnCancelProps={{ | ||
| onClick: () => close(), | ||
| }} | ||
| btnOKProps={{ | ||
| 'aria-label': 'Supprimer', | ||
| children: 'Supprimer', | ||
| onClick: () => { | ||
| deleteIndicateurPerso(); | ||
| close(); | ||
| }, | ||
| }} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Il me semble qu'on gère les modales comme tu as fait quasiment partout dans l'app, mais en terme d'architecture de composants, une suggestion serait de sortir la modale dans son propre composant indicateur.delete-modale.tsx qui contiendrait son state d'affichage ainsi que son hook de logique de suppression useDeleteIndicateurPerso.
Ça permettrait d'être plus modulaire, et de moins polluer et rendre plus lisible le composant parent.
There was a problem hiding this comment.
Je suis d'accord avec toi. Sur cette branche je me suis contentée de redécouper la page. Mais j'ai en effet extrait la modale dans son composant dédié dans la branche de refonte du header (qui viendra après).
| @@ -23,6 +23,7 @@ import FilterBadges, { | |||
| CustomFilterBadges, | |||
| useFiltersToBadges, | |||
| } from '@/app/ui/shared/filters/filter-badges'; | |||
| import _ from 'lodash'; | |||
There was a problem hiding this comment.
Plutôt utiliser es-toolkit que lodash, il y a la même fonction isEqual.
L'idée à terme c'est de supprimer totalement tous les endroits où on utilise lodash en faveur de es-toolkit, plus light et plus performant.
| filtres: Filtre; | ||
| customFilterBadges?: CustomFilterBadges; | ||
| resetFilters?: () => void; | ||
| maxNbOfCards?: number; | ||
| sortSettings?: SortFicheActionSettings; | ||
| enableGroupedActions?: boolean; | ||
| isReadOnly?: boolean; | ||
| containeClassName?: string; |
| if (!_.isEqual(filtres, filtresLocal.current)) { | ||
| filtresLocal.current = filtres; | ||
| setCurrentPage(1); | ||
| } |
There was a problem hiding this comment.
Pourquoi cette gestion du filtresLocal en plus de filtre ?
There was a problem hiding this comment.
Le fait d'avoir une copie de filtres en local me permet de comparer les filtres en props avec la valeur précédente. Etant donné que filtres est un objet, le useEffect seul ne suffit pas, et enclenche des rerender non souhaités.
Ticket : https://www.notion.so/accelerateur-transition-ecologique-ademe/Refonte-Onglets-page-indicateur-commune-1606523d57d780539af7d93421cdde37?pvs=4